Point group/select lead points#256 #277
Conversation
|
onClick the card should be green-filled as well Means that if the user clicks on the a card, like the Point 1 card, that it should be selected just as if they clicked on the select as lead button. |
|
Onclick card changes made. |
There was a problem hiding this comment.
I see that there were several changes that were requested that haven't been addressed. Is there some problem?
Also, I see that clicking on the card works for this case: http://localhost:6006/?path=/story/point-group--select-lead-points
But in these cases, clicking on the card doesn't trigger the select as lead action. -- though the card isn't expected turn green. Please make clicking on the card trigger the select action in these cases too:
http://localhost:6006/?path=/story/point-group--edit-multiple-points
http://localhost:6006/?path=/story/point-group--selected-edit-multiple-points
app/components/button.jsx
Outdated
| @@ -1,4 +1,5 @@ | |||
| // https://github.com/EnCiv/civil-pursuit/issues/43 | |||
| //https://github.com/EnCiv/civil-pursuit/issues/257 | |||
There was a problem hiding this comment.
there weren't really changes to this file, so please revert it back to what's in master.
app/components/sign-up.jsx
Outdated
| @@ -1,4 +1,5 @@ | |||
| // https://github.com/EnCiv/civil-pursuit/issues/150 | |||
| //https://github.com/EnCiv/civil-pursuit/issues/257 | |||
There was a problem hiding this comment.
this file seems to be from a different branch for a different issue. Please revert this to what's in master
app/components/point.jsx
Outdated
| @@ -85,7 +86,7 @@ const Point = forwardRef((props, ref) => { | |||
|
|
|||
| const useStylesFromThemeFunction = createUseStyles(theme => ({ | |||
| contentContainer: { | |||
| padding: '2.1875rem 1.875rem', | |||
| padding: '1rem 1.875rem 2.1875rem 1.875rem', | |||
There was a problem hiding this comment.
There are changes to this padding from another issue being worked on - https://github.com/EnCiv/civil-pursuit/pull/276/files
And they are different. For now, lets say don't make changes to point.jsx and we'll let the other issue deal with it.
|
on click card changes made. |
There was a problem hiding this comment.
Clicking on the card looks good and just a little more to go:
First, there are changes to point.jsx that have been merged into master. please go
git fetch
git merge origin/master
to pull in the changes, and if there are conflicts with the point component, prioritize the changes from master.
Then here are the issues i see, but they should be evaluated again after you merge in the other point component.
- The select as lead button doesn't appear to quite line up with the text.
- the space between cards is still small, though the space between the card and the done button looks good.
- the shadow around the cards doesn't look like it does in figma. there is shadow above and left of the card, and maybe more than expected below and to the right.
Thanks!
|
Changes made accordingly. please review it. |
There was a problem hiding this comment.
1, The horizontal space between point components has gotten bigger than it should be.
2. The button is not centered any more. Is there something about the width, margin, padding or something of the point component that is causing 1 and 2?
3. The vertical gap between the point components and the last row and the done button are not the same.
app/components/point.jsx
Outdated
| @@ -2,6 +2,7 @@ | |||
| // https://github.com/EnCiv/civil-pursuit/issues/76 | |||
There was a problem hiding this comment.
since there are no changes to this file, please revert it to origin/master:
git checkout origin/master app/components/point.jsx
git add app/components/point.jsx
|
select lead points button css changed |
|
Made changes according to comments. |
|
This is done and I am merging it, but in merging with master I've had to resolves some merge conflicts, and I've found a problem the causes some stories not tow work, that are not related to this change. So I'm working on it. But you can consider this done. And eventually it will be merged. Thanks! |
…up - but there is a discrepancy beteen point-group stories and grouping-step stories
issue 256 changes have been done. please review it.